[clang][bytecode] Fix some shift edge cases #119895

tbaederr · 2024-12-13T16:06:07Z

Around shifting negative values.

llvmbot · 2024-12-13T16:06:42Z

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Around shifting negative values.

Full diff: https://github.com/llvm/llvm-project/pull/119895.diff

3 Files Affected:

(modified) clang/lib/AST/ByteCode/Integral.h (+5-2)
(modified) clang/lib/AST/ByteCode/Interp.h (+35-33)
(modified) clang/test/AST/ByteCode/shifts.cpp (+25-16)

diff --git a/clang/lib/AST/ByteCode/Integral.h b/clang/lib/AST/ByteCode/Integral.h
index 26585799e5eadc..13fdb5369f2b7a 100644
--- a/clang/lib/AST/ByteCode/Integral.h
+++ b/clang/lib/AST/ByteCode/Integral.h
@@ -179,7 +179,10 @@ template <unsigned Bits, bool Signed> class Integral final {
   unsigned countLeadingZeros() const {
     if constexpr (!Signed)
       return llvm::countl_zero<ReprT>(V);
-    llvm_unreachable("Don't call countLeadingZeros() on signed types.");
+    if (isPositive())
+      return llvm::countl_zero<typename AsUnsigned::ReprT>(
+          static_cast<typename AsUnsigned::ReprT>(V));
+    llvm_unreachable("Don't call countLeadingZeros() on negative values.");
   }
 
   Integral truncate(unsigned TruncBits) const {
@@ -210,7 +213,7 @@ template <unsigned Bits, bool Signed> class Integral final {
     return Integral(Value.V);
   }
 
-  static Integral zero() { return from(0); }
+  static Integral zero(unsigned BitWidth = 0) { return from(0); }
 
   template <typename T> static Integral from(T Value, unsigned NumBits) {
     return Integral(Value);
diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
index cdf05e36304acb..f085f96fdf5391 100644
--- a/clang/lib/AST/ByteCode/Interp.h
+++ b/clang/lib/AST/ByteCode/Interp.h
@@ -2511,50 +2511,52 @@ inline bool DoShift(InterpState &S, CodePtr OpPC, LT &LHS, RT &RHS) {
         S, OpPC, LHS, RHS);
   }
 
-  if constexpr (Dir == ShiftDir::Left) {
-    if (LHS.isNegative() && !S.getLangOpts().CPlusPlus20) {
-      // C++11 [expr.shift]p2: A signed left shift must have a non-negative
-      // operand, and must not overflow the corresponding unsigned type.
-      // C++2a [expr.shift]p2: E1 << E2 is the unique value congruent to
-      // E1 x 2^E2 module 2^N.
-      const SourceInfo &Loc = S.Current->getSource(OpPC);
-      S.CCEDiag(Loc, diag::note_constexpr_lshift_of_negative) << LHS.toAPSInt();
-      if (!S.noteUndefinedBehavior())
-        return false;
-    }
-  }
-
   if (!CheckShift<Dir>(S, OpPC, LHS, RHS, Bits))
     return false;
 
   // Limit the shift amount to Bits - 1. If this happened,
   // it has already been diagnosed by CheckShift() above,
   // but we still need to handle it.
+  // Note that we have to be extra careful here since we're doing the shift in
+  // any case, but we need to adjust the shift amount or the way we do the shift
+  // for the potential error cases.
   typename LT::AsUnsigned R;
+  unsigned MaxShiftAmount = LHS.bitWidth() - 1;
   if constexpr (Dir == ShiftDir::Left) {
-    if (RHS > RT::from(Bits - 1, RHS.bitWidth()))
-      LT::AsUnsigned::shiftLeft(LT::AsUnsigned::from(LHS),
-                                LT::AsUnsigned::from(Bits - 1), Bits, &R);
-    else
+    if (Compare(RHS, RT::from(MaxShiftAmount, RHS.bitWidth())) ==
+        ComparisonCategoryResult::Greater) {
+      if (LHS.isNegative())
+        R = LT::AsUnsigned::zero(LHS.bitWidth());
+      else {
+        RHS = RT::from(LHS.countLeadingZeros(), RHS.bitWidth());
+        LT::AsUnsigned::shiftLeft(LT::AsUnsigned::from(LHS),
+                                  LT::AsUnsigned::from(RHS, Bits), Bits, &R);
+      }
+    } else if (LHS.isNegative()) {
+      if (LHS.isMin()) {
+        R = LT::AsUnsigned::zero(LHS.bitWidth());
+      } else {
+        // If the LHS is negative, perform the cast and invert the result.
+        typename LT::AsUnsigned LHSU = LT::AsUnsigned::from(-LHS);
+        LT::AsUnsigned::shiftLeft(LHSU, LT::AsUnsigned::from(RHS, Bits), Bits,
+                                  &R);
+        R = -R;
+      }
+    } else {
+      // The good case, a simple left shift.
       LT::AsUnsigned::shiftLeft(LT::AsUnsigned::from(LHS),
                                 LT::AsUnsigned::from(RHS, Bits), Bits, &R);
+    }
   } else {
-    if (RHS > RT::from(Bits - 1, RHS.bitWidth()))
-      LT::AsUnsigned::shiftRight(LT::AsUnsigned::from(LHS),
-                                 LT::AsUnsigned::from(Bits - 1), Bits, &R);
-    else
-      LT::AsUnsigned::shiftRight(LT::AsUnsigned::from(LHS),
-                                 LT::AsUnsigned::from(RHS, Bits), Bits, &R);
-  }
-
-  // We did the shift above as unsigned. Restore the sign bit if we need to.
-  if constexpr (Dir == ShiftDir::Right) {
-    if (LHS.isSigned() && LHS.isNegative()) {
-      typename LT::AsUnsigned SignBit;
-      LT::AsUnsigned::shiftLeft(LT::AsUnsigned::from(1, Bits),
-                                LT::AsUnsigned::from(Bits - 1, Bits), Bits,
-                                &SignBit);
-      LT::AsUnsigned::bitOr(R, SignBit, Bits, &R);
+    // Right shift.
+    if (Compare(RHS, RT::from(MaxShiftAmount, RHS.bitWidth())) ==
+        ComparisonCategoryResult::Greater) {
+      R = LT::AsUnsigned::from(-1);
+    } else {
+      // Do the shift on potentially signed LT, then convert to unsigned type.
+      LT A;
+      LT::shiftRight(LHS, LT::from(RHS, Bits), Bits, &A);
+      R = LT::AsUnsigned::from(A);
     }
   }
 
diff --git a/clang/test/AST/ByteCode/shifts.cpp b/clang/test/AST/ByteCode/shifts.cpp
index 0b3383731c6774..493f8b78204259 100644
--- a/clang/test/AST/ByteCode/shifts.cpp
+++ b/clang/test/AST/ByteCode/shifts.cpp
@@ -21,27 +21,15 @@ namespace shifts {
     c = 1 << 0;
     c = 1 << -0;
     c = 1 >> -0;
-    c = 1 << -1; // expected-warning {{shift count is negative}} \
-                 // expected-note {{negative shift count -1}} \
-                 // cxx17-note {{negative shift count -1}} \
-                 // cxx17-warning {{shift count is negative}} \
-                 // ref-warning {{shift count is negative}} \
-                 // ref-note {{negative shift count -1}} \
-                 // ref-cxx17-warning {{shift count is negative}} \
-                 // ref-cxx17-note {{negative shift count -1}}
+    c = 1 << -1; // all-warning {{shift count is negative}} \
+                 // all-note {{negative shift count -1}}
 
     c = 1 >> -1; // expected-warning {{shift count is negative}} \
                  // cxx17-warning {{shift count is negative}} \
                  // ref-warning {{shift count is negative}} \
                  // ref-cxx17-warning {{shift count is negative}}
-    c = 1 << (unsigned)-1; // expected-warning {{shift count >= width of type}} \
-                           // expected-warning {{implicit conversion from 'int' to 'char' changes value from -2147483648 to 0}} \
-                           // cxx17-warning {{shift count >= width of type}} \
-                           // cxx17-warning {{implicit conversion from 'int' to 'char' changes value from -2147483648 to 0}} \
-                           // ref-warning {{shift count >= width of type}} \
-                           // ref-warning {{implicit conversion from 'int' to 'char' changes value from -2147483648 to 0}} \
-                           // ref-cxx17-warning {{shift count >= width of type}} \
-                           // ref-cxx17-warning {{implicit conversion from 'int' to 'char' changes value from -2147483648 to 0}}
+    c = 1 << (unsigned)-1; // all-warning {{shift count >= width of type}} \
+                           // all-warning {{implicit conversion from 'int' to 'char' changes value from -2147483648 to 0}}
     c = 1 >> (unsigned)-1; // expected-warning {{shift count >= width of type}} \
                            // cxx17-warning {{shift count >= width of type}} \
                            // ref-warning {{shift count >= width of type}} \
@@ -212,3 +200,24 @@ enum shiftof {
     X3 = (1<<32) // all-error {{expression is not an integral constant expression}} \
                  // all-note {{shift count 32 >= width of type 'int'}}
 };
+
+#if __WCHAR_WIDTH__ == 32
+static_assert(((wchar_t)-1U >> 31) == -1);
+#endif
+
+#if __INT_WIDTH__ == 32
+static_assert(((int)-1U >> 32) == -1); // all-error {{not an integral constant expression}} \
+                                       // all-note {{shift count 32 >= width of type 'int' (32 bits)}}
+#endif
+
+static_assert((-4 << 32) == 0); // all-error {{not an integral constant expression}} \
+                                // all-note {{shift count}}
+
+static_assert((-4 << 1) == -8); // ref-cxx17-error {{not an integral constant expression}} \
+                                // ref-cxx17-note {{left shift of negative value -4}} \
+                                // cxx17-error {{not an integral constant expression}} \
+                                // cxx17-note {{left shift of negative value -4}}
+static_assert((-4 << 31) == 0); // ref-cxx17-error {{not an integral constant expression}} \
+                                // ref-cxx17-note {{left shift of negative value -4}} \
+                                // cxx17-error {{not an integral constant expression}} \
+                                // cxx17-note {{left shift of negative value -4}}

llvm-ci · 2024-12-14T05:25:48Z

LLVM Buildbot has detected a new failure on builder clang-aarch64-quick running on linaro-clang-aarch64-quick while building clang at step 5 "ninja check 1".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/9344